Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add watchdog process info as cleanup argument #59

Merged
merged 1 commit into from
Aug 7, 2024
Merged

feat: add watchdog process info as cleanup argument #59

merged 1 commit into from
Aug 7, 2024

Conversation

frattaro
Copy link
Contributor

In regards to #58 -- this PR sends the watchdog process info as a third argument to the cleanup function.

All I really need is the watchdog pid but I figured more info is more helpful. You could probably put the other process info in here too but I don't need it.

@frattaro frattaro mentioned this pull request Jul 15, 2024
Merged
@isaacs
Copy link
Member

isaacs commented Jul 23, 2024

Can you help me understand why this is needed? The watchdog process ends on its own when the fg child proc ends.

@frattaro
Copy link
Contributor Author

frattaro commented Jul 23, 2024 •
edited
Loading

nyc uses process information during generation of files/directories for storing coverage data. Because the watchdog was introduced, it's producing additional empty files. If I have the pid of the watchdog I can identify the junk data it's generating and deal with it (clean it up afterwards -- wouldn't be able to ignore it during since it's not identifiable until it's run)

And the additional files result in things like "Unknown % coverage"

@frattaro
Copy link
Contributor Author

frattaro commented Aug 5, 2024

@isaacs does that clear it up?

@isaacs
Copy link
Member

isaacs commented Aug 7, 2024

It seems a bit risky to share the ChildProcess object itself, tempting to try to interact with, when it's already closed and really nothing can be done about it. Would it be sufficient for your purpose to just share the pid?

@frattaro
Copy link
Contributor Author

frattaro commented Aug 7, 2024

Yeah pid only absolutely. I'll trim it up

isaacs pushed a commit to frattaro/foreground-child that referenced this pull request Aug 7, 2024
@frattaro @isaacs
feat: add watchdog pid as cleanup metadata arg
2939728 
PR-URL: tapjs#59
Credit: @frattaro
Close: tapjs#59
Reviewed-by: @isaacs
isaacs pushed a commit to frattaro/foreground-child that referenced this pull request Aug 7, 2024
@frattaro @isaacs
feat: add watchdog pid as cleanup metadata arg
1b9aedd 
PR-URL: tapjs#59
Credit: @frattaro
Close: tapjs#59
Reviewed-by: @isaacs
@isaacs isaacs merged commit aa2cba8 into tapjs:main Aug 7, 2024
6 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@frattaro @isaacs